CLN: Flake8 E741 #22913

alimcmaster1 · 2018-09-30T19:20:07Z

towards STYLE: Additional flake8 checks #22122 ( test files still remaining for E741)
passes git diff upstream/master -u -- "*.py" | flake8 --diff

Import optimiser, makes things alphabetical and also removes unused eg, Float64Index, in packers.py. Do people prefer this or not?

pep8speaks · 2018-09-30T19:20:16Z

Hello @alimcmaster1! Thanks for submitting the PR.

There are no PEP8 issues in the file pandas/core/frame.py !
There are no PEP8 issues in the file pandas/core/indexes/range.py !
There are no PEP8 issues in the file pandas/core/indexing.py !
There are no PEP8 issues in the file pandas/io/packers.py !
There are no PEP8 issues in the file pandas/io/pytables.py !

datapythonista · 2018-09-30T19:39:25Z

I'm personally happy with this. @jreback ?

I'd probably use length instead of l or ln, but other than that, the changes look all right.

Once #22863 is merged, it'll be good to stop ignoring E402 and E741 from the linting, and see if there is much more being reported.

jbrockmendel · 2018-09-30T19:56:09Z

pandas/core/frame.py

+from pandas import compat
+from pandas._libs import lib, algos as libalgos
+from pandas.compat import PY36
+from pandas.compat import (range, map, zip, lrange, lmap, lzip, StringIO, u,


Combine with previous

Thanks done!

This is great, thanks, I know its nitpicky. For future reference in case you want to really match my habits, I usually put a newline after the compat imports, then collect core.dtypes imports in a section above the rest of core (lots of stuff depends on core.dtypes, but it depends on very little except for compat and _libs)

Thanks much appreciated, I will update! I might add some kind of description to the contributing guide on the import layout you have described here, unless its already documented somewhere?

jbrockmendel · 2018-09-30T19:57:59Z

pandas/core/frame.py

+import pandas.io.formats.format as fmt
+import pandas.plotting._core as gfx
+from pandas import compat
+from pandas._libs import lib, algos as libalgos


Internal convention is to group by dependency structure. This usually means libs first, then util, compat, then core, then ...

Actually the pytables module is a pretty good example

Cool thanks for the example @jbrockmendel will fix this up!

jbrockmendel · 2018-09-30T19:58:46Z

pandas/core/frame.py

-
-from pandas.core.config import get_option
+# pylint: disable=E1101,E1103
+# pylint: disable=W0212,W0231,W0703,W0622


Any idea what these are and if they’re still needed?

Good point I removed E1103,W0231 as they are no longer required:

"E1101: frame.py:2577:40: E1101: Module 'pandas.core.common' has no '_unpickle_array' member (no-member)"

"W0622: frame.py:36:0: W0622: Redefining built-in 'zip' (redefined-builtin)"

I guess we should really import like so: from pandas.compat import range as compatrange?

alimcmaster1 · 2018-10-02T23:45:24Z

Thanks for taking a look @datapythonista updated as per your comments. ln -> length is clearer to me too :)

pandas/core/frame.py

jbrockmendel · 2018-10-02T23:58:37Z

pandas/core/indexes/range.py

@@ -512,33 +512,33 @@ def __getitem__(self, key):
            # This is basically PySlice_GetIndicesEx, but delegation to our
            # super routines if we don't have integers

-            l = len(self)
+            length = len(self)


+1 good choice

jbrockmendel · 2018-10-02T23:59:14Z

pandas/core/indexing.py

@@ -305,7 +305,7 @@ def _setitem_with_indexer(self, indexer, value):

        # also has the side effect of consolidating in-place
        # TODO: Panel, DataFrame are not imported, remove?
-        from pandas import Panel, DataFrame, Series  # noqa
+        from pandas import Series  # noqa


I think the noqa is now unnecessary

jbrockmendel · 2018-10-02T23:59:38Z

pandas/core/indexing.py

        elif stop < 0:
-            stop += l
+            stop += target_len


+1 on all the changed names here; this is very clear

pandas/io/packers.py

jbrockmendel · 2018-10-03T00:01:16Z

pandas/io/packers.py

 from pandas.core.sparse.api import SparseSeries, SparseDataFrame
 from pandas.core.sparse.array import BlockIndex, IntIndex
-from pandas.core.generic import NDFrame
 from pandas.errors import PerformanceWarning


pandas.errors is among the zero-dependency directories id put somewhere around util/compat

Updated thanks!

jbrockmendel · 2018-10-03T00:02:02Z

pandas/io/pytables.py

 from pandas._libs import algos, lib, writers as libwriters
 from pandas._libs.tslibs import timezones
-
-from pandas.errors import PerformanceWarning
-from pandas import compat
 from pandas.compat import u_safe as u, PY3, range, lrange, string_types, filter


u is no longer needed, can be replaced by u literal

Replaced all occurrences with the literal for this file

TomAugspurger · 2018-10-03T02:40:04Z

In general, I'm in favor of sorting imports, but we shouldn't be doing manually and it should be linted, else we'll be doing it again in a few months.

I'd recommend getting a consensus on an isort configuration that we all like, and then adding a lint check to ensure that the sort order is respected.

isort --recursive --check-only .

For dask-ml we use

[isort]
multi_line_output=3
include_trailing_comma=True
force_grid_wrap=0
combine_as_imports=True
line_length=88

alimcmaster1 · 2018-10-03T20:48:44Z

Thanks @TomAugspurger totally agree, let me create an issue and see if we can decide on config set up.

codecov · 2018-10-08T18:51:24Z

Codecov Report

❗ No coverage uploaded for pull request base (master@e510b1a). Click here to learn what that means.
The diff coverage is 95.48%.

@@            Coverage Diff            @@
##             master   #22913   +/-   ##
=========================================
  Coverage          ?   92.19%           
=========================================
  Files             ?      169           
  Lines             ?    50879           
  Branches          ?        0           
=========================================
  Hits              ?    46910           
  Misses            ?     3969           
  Partials          ?        0

Flag	Coverage Δ
#multiple	`90.61% <86.45%> (?)`
#single	`42.32% <89.03%> (?)`

Impacted Files	Coverage Δ
pandas/core/frame.py	`97.2% <100%> (ø)`
pandas/core/indexing.py	`93.87% <100%> (ø)`
pandas/core/indexes/range.py	`95.73% <100%> (ø)`
pandas/io/packers.py	`88.04% <92.3%> (ø)`
pandas/io/pytables.py	`92.44% <93.61%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e510b1a...04478f9. Read the comment docs.

alimcmaster1 · 2018-10-08T20:00:37Z

Thanks @jbrockmendel for the review, ive updated as per your comments mind confirming this looks good?

jbrockmendel · 2018-10-08T23:00:26Z

I think all my nitpickings have been addressed.

datapythonista

lgtm, great work.

It'd be nice at some point to get rid of the custom disables in the linting, but if that can be done, it should be in a separate PR

jreback · 2018-10-09T11:41:08Z

pandas/core/frame.py

@@ -1,3 +1,5 @@
+# pylint: disable=E1101
+# pylint: disable=W0212,W0703,W0622


i would rather not disable specific items for a whole file. Is there a reason for this?

@jreback 100% agree, but these lines are already there in the original files, they've just been moved.

@alimcmaster1 removed some of the errors that were being ignored, but the ones left are not trivial.

Didn't check in detail, but E1101 is probably to avoid false positives in attributes that are created dynamically and not found in the linting. It's disabled like this in around 80 files, so it may be worth to move it to setup.cfg and ignore it everywhere.

Didn't check the others, but I'd merge this PR as it is, and take care of all the pylint: disable in a separate PR.

ok that's fine.

Thanks both, had a brief discussion above with @jbrockmendel about these.

"E1101: frame.py:2577:40: E1101: Module 'pandas.core.common' has no '_unpickle_array' member (no-member)" this is as @datapythonista described above.

"W0622: frame.py:36:0: W0622: Redefining built-in 'zip' (redefined-builtin)"
We should be able to get rid of W0622, I can do a follow up PR with this.

I will follow and remove E741 from our lint CI setup and fix this error for the few remaining test scripts.

I'm merging #22863 as soon as the CI is green, so you may want to wait to remove E741, as it'll conflict.

Merged. E741 should be removed from setup.cfg and .pep8speaks.yml. But not sure if the remaining tests are few. ;)

./pandas/tests/test_algos.py:375:9: E741 ambiguous variable name 'l' ./pandas/tests/test_multilevel.py:2065:9: E741 ambiguous variable name 'l' ./pandas/tests/frame/test_operators.py:796:9: E741 ambiguous variable name 'l' ./pandas/tests/frame/test_indexing.py:2079:9: E741 ambiguous variable name 'l' ./pandas/tests/frame/test_constructors.py:899:9: E741 ambiguous variable name 'l' ./pandas/tests/frame/test_constructors.py:926:9: E741 ambiguous variable name 'l' ./pandas/tests/frame/test_constructors.py:1747:9: E741 ambiguous variable name 'l' ./pandas/tests/frame/test_constructors.py:1752:9: E741 ambiguous variable name 'l' ./pandas/tests/plotting/common.py:260:24: E741 ambiguous variable name 'l' ./pandas/tests/plotting/test_frame.py:302:9: E741 ambiguous variable name 'l' ./pandas/tests/plotting/test_frame.py:310:9: E741 ambiguous variable name 'l' ./pandas/tests/plotting/test_datetimelike.py:545:9: E741 ambiguous variable name 'l' ./pandas/tests/plotting/test_datetimelike.py:560:9: E741 ambiguous variable name 'l' ./pandas/tests/plotting/test_datetimelike.py:575:9: E741 ambiguous variable name 'l' ./pandas/tests/plotting/test_datetimelike.py:595:9: E741 ambiguous variable name 'l' ./pandas/tests/plotting/test_datetimelike.py:611:9: E741 ambiguous variable name 'l' ./pandas/tests/plotting/test_datetimelike.py:642:9: E741 ambiguous variable name 'l' ./pandas/tests/plotting/test_datetimelike.py:953:17: E741 ambiguous variable name 'l' ./pandas/tests/plotting/test_datetimelike.py:966:17: E741 ambiguous variable name 'l' ./pandas/tests/plotting/test_datetimelike.py:984:17: E741 ambiguous variable name 'l' ./pandas/tests/plotting/test_datetimelike.py:998:17: E741 ambiguous variable name 'l' ./pandas/tests/indexing/test_loc.py:671:13: E741 ambiguous variable name 'l' ./pandas/tests/indexing/test_indexing.py:772:19: E741 ambiguous variable name 'l' ./pandas/tests/io/test_packers.py:515:9: E741 ambiguous variable name 'l' ./pandas/tests/io/test_packers.py:521:9: E741 ambiguous variable name 'l' ./pandas/tests/io/test_packers.py:529:9: E741 ambiguous variable name 'l' ./pandas/tests/io/test_pytables.py:2185:13: E741 ambiguous variable name 'l' ./pandas/tests/series/test_analytics.py:542:13: E741 ambiguous variable name 'l' ./pandas/tests/series/test_analytics.py:977:13: E741 ambiguous variable name 'l' ./pandas/tests/series/test_dtypes.py:246:9: E741 ambiguous variable name 'l' ./pandas/tests/series/test_dtypes.py:252:9: E741 ambiguous variable name 'l' ./pandas/tests/series/test_dtypes.py:273:9: E741 ambiguous variable name 'l'

jreback · 2018-10-09T11:42:04Z

pandas/core/indexing.py

@@ -1,10 +1,16 @@
 # pylint: disable=W0223


can you remove this warning? The general problem with these warnings is that they are added, but never removed.

that's fine too

jreback · 2018-10-09T11:43:16Z

If you want to add checking for specific files for certain warnings, then add a line to lint.sh to do this.

jreback · 2018-10-09T15:44:02Z

thanks @alimcmaster1

just want point out that we need a good way of validating import ordering before we wholesale fix it.

alimcmaster1 · 2018-10-09T18:42:15Z

thanks @jreback for the review. Ive raised an issue for this here #23048 , to clarify: by validating import ordering are you referring to the fact we need to group by dependency structure like @jbrockmendel is referring to here? Or am I missing something here?

datapythonista added CI Continuous Integration Code Style Code style, linting, code_checks labels Sep 30, 2018

jbrockmendel reviewed Sep 30, 2018

View reviewed changes

jbrockmendel reviewed Oct 2, 2018

View reviewed changes

pandas/core/frame.py Show resolved Hide resolved

jbrockmendel reviewed Oct 2, 2018

View reviewed changes

pandas/core/indexing.py

elif stop < 0:

stop += l

stop += target_len

Copy link

Member

jbrockmendel Oct 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 on all the changed names here; this is very clear

jbrockmendel reviewed Oct 3, 2018

View reviewed changes

pandas/io/packers.py Outdated Show resolved Hide resolved

jbrockmendel reviewed Oct 3, 2018

View reviewed changes

alimcmaster1 changed the title ~~CLN: Flake8 E741~~ WIP: Flake8 E741 Oct 3, 2018

alimcmaster1 added 7 commits October 8, 2018 19:46

Flake8 E741

ed8fbbb

E741 update as per comments

0779dfb

Remove pylint

869ca05

Add back gfx import

9c72397

Use u literal

9474944

Flake8 ignore multiple import on one line

76c1f8f

Fix import ordering

04478f9

alimcmaster1 mentioned this pull request Oct 8, 2018

Use isort for standardizing import formatting #23048

Closed

alimcmaster1 changed the title ~~WIP: Flake8 E741~~ CLN: Flake8 E741 Oct 8, 2018

datapythonista approved these changes Oct 9, 2018

View reviewed changes

jreback added this to the 0.24.0 milestone Oct 9, 2018

jreback requested changes Oct 9, 2018

View reviewed changes

jreback approved these changes Oct 9, 2018

View reviewed changes

jreback merged commit ca7d518 into pandas-dev:master Oct 9, 2018

alimcmaster1 deleted the E741 branch October 9, 2018 18:42

alimcmaster1 mentioned this pull request Oct 13, 2018

CLN: Flake8 E741 #23131

Merged

1 task

tm9k1 pushed a commit to tm9k1/pandas that referenced this pull request Nov 19, 2018

CLN: Flake8 E741 (pandas-dev#22913)

2d7b58b

		@@ -1,3 +1,5 @@
		# pylint: disable=E1101
		# pylint: disable=W0212,W0703,W0622

CLN: Flake8 E741 #22913

CLN: Flake8 E741 #22913

Conversation

alimcmaster1 commented Sep 30, 2018

pep8speaks commented Sep 30, 2018

datapythonista commented Sep 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alimcmaster1 Oct 2, 2018 • edited Loading

Choose a reason for hiding this comment

alimcmaster1 commented Oct 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alimcmaster1 Oct 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomAugspurger commented Oct 3, 2018

alimcmaster1 commented Oct 3, 2018

codecov bot commented Oct 8, 2018 • edited Loading

Codecov Report

alimcmaster1 commented Oct 8, 2018

jbrockmendel commented Oct 8, 2018

datapythonista left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

datapythonista Oct 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented Oct 9, 2018

jreback commented Oct 9, 2018

alimcmaster1 commented Oct 9, 2018

alimcmaster1 Oct 2, 2018 •

edited

Loading

alimcmaster1 Oct 8, 2018 •

edited

Loading

codecov bot commented Oct 8, 2018 •

edited

Loading

datapythonista Oct 9, 2018 •

edited

Loading